-
Notifications
You must be signed in to change notification settings - Fork 300
Add custom feature creation documentation #1295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Add custom feature creation documentation #1295
Conversation
Qodana for JVM1254 new problems were found
@@ Code coverage @@
+ 72% total lines covered
18063 lines analyzed, 13017 lines covered
# Calculated according to the filters of your coverage tool☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
| - `interceptSubgraphExecutionCompleted`: After a subgraph completes. | ||
| - `interceptSubgraphExecutionFailed`: When a subgraph execution fails. | ||
|
|
||
| Note that interceptors are feature-scoped: only the feature that registers a handler receives those events (subject to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a note?) Also, I am lost at this point. You never mentioned handlers before this point. Event handlers are a predefined feature, so I am not sure if that's what you mean. The FeatureConfig subclass in your example doesn't have setEventFilter overridden. If this is an important note, I'd expand on this, provide some context and links.
| ### Disabling event filtering for a feature | ||
|
|
||
| Some features, such as debugger and OpenTelemetry, must observe the entire event stream. If your feature depends on the | ||
| full event stream, disable event filtering by overriding `setEventFilter` in your feature configuration to ignore custom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you mention setEventFilter again, but it is not at all evident what it's for and why you need to make it always return true (disable events filtering) if the feature "depends on the full event stream". Are some events filtered by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there are default filters, but you should be able to set your own ones when installing a feature in an agent. As some features, such as OpenTelemetry, may depend on receiving all events as they are, this is a way to prevent any filtering or processing that would interfere with the functioning of the feature. @sdubov can confirm whether my understanding here is correct.
I agree we can make this more explicit, though.
| ```kotlin | ||
| class LoggingFeature(private val logger: KLogger) { | ||
| class Config : FeatureConfig() { | ||
| var loggerName: String = "agent-logs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this loggerName if it's not used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be likely related to the comment below, illustrated in a new example that shows how an agent is created with the installed custom feature.
| override fun createInitialConfig(): Config = Config() | ||
|
|
||
| override fun install(config: Config, pipeline: AIAgentGraphPipeline) : LoggingFeature { | ||
| val logging = LoggingFeature(logger = logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this logger come from that you pass as the logger argument in the constructor?
| } | ||
| } | ||
| ``` | ||
| <!--- KNIT example-custom-features-04.kt --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to show how to install the feature, or is it straightforward? In the first example, you pass a string to the property when installing the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, I'm not sure whether feature installation as an individual concept is discoverable enough, e.g. from here. It may make sense to show how this specific feature is installed, along with the use of loggerName, which may also clarify things related to comments above.
Add documentation on how to implement custom features.
Type of the changes
Checklist
developas the base branchAdditional steps for pull requests adding a new feature